Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify correct terminal to start the packager #24436

Conversation

lucasbento
Copy link
Contributor

Summary

This PR specifies the correct terminal to start the Metro bundler as stated in react-native-community/cli#303.

Changelog

[iOS] [Changed] - Start the packager on the correct terminal.

Test Plan

To make sure that this works just run it from within Xcode and it should go to your default terminal, you can also try setting the REACT_TERMINAL environment variable to make sure that the packager will open on the desired terminal.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 13, 2019
@react-native-bot react-native-bot added Tech: Bundler 📦 This issue is related to the bundler (Metro, Haul, etc) used. Includes Changelog labels Apr 13, 2019
@thymikee
Copy link
Contributor

I actually couldn't get this to work (legacy build, fish shell, latest macOS). Turns out that printenv inside Xcode results in completely different envs and TERM_PROGRAM is not there. Ideas?

@lucasbento
Copy link
Contributor Author

@thymikee: what happens in the end?

I'm on macOS 10.14, Hyper installed, legacy build it is working here, maybe Hyper did set it up.

I also trying running the app from fishshell.com and it has the TERM_PROGRAM on it, not entirely sure how I can reproduce your issue.

@thymikee
Copy link
Contributor

It doesn't open the terminal at all. I'm really not sure why that happens. I use iTerm. The TERM_PROGRAM is there on fish, zsh, bash and sh, just Xcode doesn't see it on my machine. If it works for you, I think it would be fine to default the $terminal to Terminal.app so it doesn't regress.

@lucasbento
Copy link
Contributor Author

Strange. That's a good idea though, I'll do it.

@noahtallen
Copy link

Nifty! I'm curious - I just opened this issue on the CLI repo. Essentially, I have react imported through Pods, and not at all through the Xcode project. Does this mean that this shellScript would not get run? Because it currently does not launch the packager for me on react-native run-ios for a simulator (though it appears that normal bundling during a release build works fine).

@lucasbento
Copy link
Contributor Author

@noahtallen: let's continue the conversation on react-native-community/cli#317, I would like to fix that problem.

@lucasbento
Copy link
Contributor Author

@thymikee: I just added a check to verify if it found the default terminal or not, if not just open the packager without specifying it.

@lucasbento lucasbento force-pushed the feature/correct-terminal-metro-bundler branch from 877ed5a to 2ecbc0c Compare April 15, 2019 07:32
Copy link
Contributor

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After fixing syntax errors (that I see now are fixed :D), when building from command line (run-ios) my default terminal launches, but if I build directly from Xcode, the system Terminal.app is opened.

I googled a bit and I found this:
https://forums.developer.apple.com/thread/8451

Looks like the default for UseSanitizedBuildSystemEnvironment arg for Xcode IDE is YES, and it's the opposite when running xcodebuild and hence the difference. Not sure if there's anything we can do about that other than document it?

@lucasbento
Copy link
Contributor Author

@thymikee: sorry for the syntax error, thought I was testing this but in the end was testing another app.

I tried to check if I have UseSanitizedBuildSystemEnvironment set to NO but apparently I don't (I also don't recall setting it) and the Xcode still inherits the env variables.

If you define it as NO, does it work?

I'll try to check with a co-worker to see if I can reproduce that on their computer as well.

@lucasbento
Copy link
Contributor Author

lucasbento commented Apr 15, 2019

Btw I checked with defaults read-type com.apple.dt.Xcode UseSanitizedBuildSystemEnvironment not sure if this is the correct syntax.

@thymikee
Copy link
Contributor

Just tried and still nope (inb4 I restarted Xcode)

@lucasbento
Copy link
Contributor Author

I'll check it here with some of my co-workers to see if I can reproduce that problem.

@thymikee
Copy link
Contributor

I've additionally tried this on a fresh project without pods, with UseSanitizedBuildSystemEnvironment disabled (and enabled as well). Still same results. Wonder if I set something weird years ago and now don't remember :D

Copy link
Contributor

@michalchudziak michalchudziak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@lucasbento
Copy link
Contributor Author

@thymikee: I tested on two other MacBooks and it's the same behaviour as yours, TERM_PROGRAM is not defined for some reason, so I guess I'm the one that may have done something weird in my machine to have it.

I'm wondering now if there's any way that we can pass the TERM_PROGRAM to the build phase script otherwise this will work only for a few people (I assume) and for the ones that use the CLI.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @lucasbento in 3273d23.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Apr 15, 2019
@lucasbento
Copy link
Contributor Author

@thymikee: the only way I see atm to improve this part is to have a way to customize those env variables through a file so that the script can read from it instead of trying to get the global ones, unfortunately this PR will not change the behaviour for people that use Xcode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Tech: Bundler 📦 This issue is related to the bundler (Metro, Haul, etc) used.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants